fix(rateLimit): exempt all relation DELETE routes from IP rate limiter#1710
Conversation
- Extend skip logic to cover all 10 relation/association DELETE endpoints - Add RELATION_DELETE_PATTERNS array for maintainability - Add authenticated user test variant (mirrors real call path) - Add negative assertion test for paths extending beyond relation routes - Fix comment accuracy (tokenAuth = authentication, controller = ownership) Closes #1707
Paul-AUB
left a comment
Paul-AUB
left a comment
There was a problem hiding this comment.
Solid, well-scoped change. All 10 RELATION_DELETE_PATTERNS were cross-checked against config/routes.js and each one maps to a real DELETE route with matching parameter arity. Since deleteRateLimit runs as a global middleware (config/http.js order array), req.path correctly resolves to the full /api/v1/... path, so the anchored regexes behave as intended. The new tests are a good improvement (authenticated variant + negative boundary assertion). No blocking issues found.
Suggestions (Should Consider)
-
Regression coverage for destructive DELETEs —
config/rateLimit/rateLimiter.js:109-120The new patterns correctly don't match single-segment destructive routes (e.g.DELETE /api/v1/entrances/:id), and the/extranegative test is a nice guard. Consider also adding one assertion that a genuinely destructive DELETE path is still rate-limited (returns 429), so a future broadening ofRELATION_DELETE_PATTERNScan't silently exempt a destructive route. -
Pattern/route drift —
RELATION_DELETE_PATTERNSduplicates the relation-route list that also lives inconfig/routes.js. As relation endpoints are added, the two can drift (a new relation route would silently inherit the strict 1/h limit until someone remembers to update this array). Worth a short comment pointing maintainers here, or a follow-up to derive the list from a single source.
Nitpicks (Optional)
-
Trailing slash —
config/rateLimit/rateLimiter.js:110-119The$-anchored patterns won't match a trailing-slash variant (e.g./api/v1/caves/1/documents/50/), which would fall through to the strict limiter. Real-world risk is low (clients don't send trailing slashes here), just noting the edge. -
Test setup duplication —
test/integration/2_utils/rateLimiter.test.js:The unauthenticated and authenticated exemption tests share most of their app/agent setup and the sameRELATION_PATHSloop. Could be parameterized to reduce duplication, but it's perfectly readable as-is.
- Add regression test for destructive DELETE route still being rate-limited - Add MAINTENANCE comment warning about pattern/route drift
|
Thanks for the thorough review @Paul-AUB! Addressed in 8a021f0: Suggestion 2 (Regression coverage): Added a test asserting that Suggestion 3 (Pattern/route drift): Added a Suggestions 4 & 5 (trailing slash, test duplication): Acknowledged but skipped — low risk and readable as-is per your own notes. |
2 participants
🤔 What
deleteRateLimitmiddleware.🤷♂️ Why
The
deleteRateLimitallows regular users only 1 DELETE request per hour per IP. This is far too aggressive for endpoints that only remove an association row (e.g., unlinking a document from an entrance, removing a caver from an organization). These endpoints don't destroy data — they remove a link between two existing entities. A user managing their associations could exhaust their hourly quota with a single action.🔍 How
Added a
RELATION_DELETE_PATTERNSarray listing all 10 relation DELETE path regexes, and anisRelationDelete()helper used in thedeleteRateLimit.skipfunction. When a request path matches any of the patterns, the destructive-action rate limit is skipped. ThegeneralRateLimitstill applies to these routes, so they're not unprotected.Also addressed Paul's review suggestions from #1706:
req.tokenset (regular user) to mirror the real authenticated call path./api/v1/entrances/42/cavers/7/extra(extending beyond the pattern) are still rate-limited (429).🧪 Testing
All 15 rate limiter tests pass, including the 3 new ones.
📸 Previews
N/A